-
Notifications
You must be signed in to change notification settings - Fork 4
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Separators: Implement html chunking strategy. #23
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This looks great: one question, but no blockers. Thank you again!
:rtf, | ||
:ruby, | ||
:typescript, | ||
:vue |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you for organising these!
"<article", | ||
"<section", | ||
"<table" | ||
] ++ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm curious, why is it splitting on <h1
rather than <h1>
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I was just being lazy and following the other examples (vue).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It just struck me while driving that this will also split on any HMTL tags that have attrs! So it makes way more sense than adding in the closing >
.
end | ||
|
||
def get_separators(:typescript), do: get_separators(:javascript) | ||
|
||
def get_separators(format) when format in @plaintext_formats, do: get_separators(:plaintext) | ||
|
||
defp empty_and_new_line_separators do |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nice
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is great, thank you!
My only inclination to nit-pick is around whether <article
and <section
should be higher-up in the separator list. But the more I think about it, I'm concluding that documents that use those elements according to the spec are most likely also going to be using the <h...
tags in a way that gives us better information about how to group the content.
tl;dr; LGTM
I actually thought about not including article and section - perhaps they are actually unnecessary. Thoughts? If you think we should keep, moving up prob makes sense. If so - where? |
@cpursley I can't convince myself that moving (or removing) them would be a definite improvement, so I'm happy with this until somebody comes along with a stronger argument 😄 Thanks so much for this contribution! |
You're welcome. Y'all should do a detailed blog post about RAG stuff in Elixir (how you're working with the chunked data, etc)! |
No description provided.